Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add storage_encrypted as an optional parameter to aws_rds_cluster #5520

Merged
merged 5 commits into from
Mar 9, 2016

Conversation

bsiegel
Copy link

@bsiegel bsiegel commented Mar 8, 2016

Provides a basic implementation of #4414, matching the current implementation for aws_db_instance added in #1041. This basic implementation allows the user to set the value of the StorageEncrypted flag at creation time, but does not allow customizing the KMS parameters as requested in #4414. I feel like that should be done in a separate PR, so that it can be added in a uniform way to all AWS resources that can accept KMS parameters.

Brandon Siegel added 2 commits March 8, 2016 16:48
@@ -71,6 +71,12 @@ func resourceAwsRDSCluster() *schema.Resource {
Computed: true,
},

"storage_encrypted": &schema.Schema{
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The documentation update suggests that the default for this value is false - I think we need to actually have this in the schema as follows:

"storage_encrypted": &schema.Schema{
         Type:     schema.TypeBool,
    Optional: true,
        Default: false,
    ForceNew: true,
}

thoughts?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The default is false if the parameter is omitted from the request, so I see two options. I could either update the schema to include the default value, or I could only set the StorageEncrypted value on the CreateDBClusterInput object if a value is provided. Which do you think is best?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

personally, I would suggest to be explicit in the terraform schema. This will safeguard against changes in AWS behaviour :)

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds good! Fixed in b3864db

@bsiegel
Copy link
Author

bsiegel commented Mar 8, 2016

I wasn't sure about the interplay between the values of StorageEncrypted on the cluster itself vs. the cluster instances, until I located this in the AWS CloudFormation user guide:

StorageEncrypted

Indicates whether the database instance is encrypted.

If you specify the DBClusterIdentifier, DBSnapshotIdentifier, or SourceDBInstanceIdentifier property, do not specify this property. The value is inherited from the cluster, snapshot, or source database instance.

@stack72
Copy link
Contributor

stack72 commented Mar 8, 2016

@bsiegel I think we need to add some tests to make sure this works as expected when used in combination with these properties. I think these limitations should then be added to the docs

@bsiegel
Copy link
Author

bsiegel commented Mar 8, 2016

I think we need to add some tests to make sure this works as expected when used in combination with these properties. I think these limitations should then be added to the docs

These limitations are on the DBInstance itself, not on the cluster. In this case, the DBInstance is exposed via the aws_rds_cluster_instance resource which does not permit specifying a storage_encrypted flag.

I would definitely be up for adding some tests though. I'll see what I can come up with.

@bsiegel
Copy link
Author

bsiegel commented Mar 8, 2016

@stack72 Added some tests, let me know if you think those are sufficient.

@stack72
Copy link
Contributor

stack72 commented Mar 9, 2016

Hi @bsiegel - running the TestAccAWSRDSCluster now and pending they all pass, will merge :) Thanks for the PR here

@stack72
Copy link
Contributor

stack72 commented Mar 9, 2016

make testacc TEST=./builtin/providers/aws TESTARGS='-run=TestAccAWSRDSCluster' 2>~/tf.log
==> Checking that code complies with gofmt requirements...
go generate $(go list ./... | grep -v /vendor/)
TF_ACC=1 go test ./builtin/providers/aws -v -run=TestAccAWSRDSCluster -timeout 120m
=== RUN   TestAccAWSRDSClusterInstance_basic
--- PASS: TestAccAWSRDSClusterInstance_basic (731.41s)
=== RUN   TestAccAWSRDSCluster_basic
--- PASS: TestAccAWSRDSCluster_basic (125.26s)
=== RUN   TestAccAWSRDSCluster_encrypted
--- PASS: TestAccAWSRDSCluster_encrypted (115.32s)
=== RUN   TestAccAWSRDSCluster_backupsUpdate
--- PASS: TestAccAWSRDSCluster_backupsUpdate (126.96s)
PASS
ok      github.com/hashicorp/terraform/builtin/providers/aws    1098.973s

stack72 added a commit that referenced this pull request Mar 9, 2016
Add storage_encrypted as an optional parameter to aws_rds_cluster
@stack72 stack72 merged commit b5e6cb5 into hashicorp:master Mar 9, 2016
@ghost
Copy link

ghost commented Apr 27, 2020

I'm going to lock this issue because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues.

If you have found a problem that seems similar to this, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

@ghost ghost locked and limited conversation to collaborators Apr 27, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants